-
Notifications
You must be signed in to change notification settings - Fork 182
feat: container info sidebar and add sidebar updates #2830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: container info sidebar and add sidebar updates #2830
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
5f8ac0b to
feba274
Compare
9f4a330 to
4f29a90
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2830 +/- ##
========================================
Coverage 95.21% 95.21%
========================================
Files 1318 1326 +8
Lines 30018 30343 +325
Branches 6544 6639 +95
========================================
+ Hits 28582 28892 +310
- Misses 1379 1394 +15
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3ceae1c to
5590401
Compare
2cc17aa to
e685ab3
Compare
rpenido
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, awesome work here @navinkarkera!
Thanks for all the refactors! After this PR, we will be way closer to saying goodbye to our redux store.
I asked a few clarifying questions about our invalidation strategy.
Everything worked as expected, except for a small bug with the taxonomy button on the header.
| iconAs={EditIcon} | ||
| onClick={onClickEdit} | ||
| onClick={onEditClick} | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was fixed upstream
| // @ts-ignore |
src/course-outline/data/api.ts
Outdated
| * @param {string} itemId | ||
| * @returns {Promise<Object>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this anymore.
| * @param {string} itemId | |
| * @returns {Promise<Object>} |
| await callback?.(data.locator, variables.parentLocator); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.parentLocator) }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseDetails(getCourseKey(data.locator)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to invalidate the courseDetails here? Do you know which data it sync?
Maybe this is fixing a sync issue by its side effects: refetching the course details would cause a re-render, which will resync the data.
It would be preferable that we find the root cause, or, if it is starting to become complicated, just invalidate the whole course with courseOutlineQueryKeys.course(getCourseKey(data.locator)),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido We invalidate courseDetails to update the course sync status which is displayed in status bar in course outline. I am not sure what you mean by re-render but this api is quiet small and only used in fetching course metadata (not the whole course outline).
src/course-outline/data/apiHooks.ts
Outdated
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(courseId) }); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.itemId) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to invalidate anything besides containerComparisonQueryKeys.course(courseId) here, since they all have the same predicate.
Could you check it?
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(courseId) }); | |
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(variables.itemId) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, courseOutlineQueryKeys.courseDetails is different from courseOutlineQueryKeys.course and courseOutlineQueryKeys.courseItem. They are related to fetching outline data while course details will just fetch course metadata.
If we invalidate courseOutlineQueryKeys.course(courseId), it will invalidate all course item queries unnecessarily. We only need to invalidate the item itself and the course metadata to update status bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I just noted that containerComparisonQueryKeys.course was a different key.
src/course-outline/data/apiHooks.ts
Outdated
| mutationFn: publishCourseItem, | ||
| onSettled: async (_data, _err, itemId) => { | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseItemId(itemId) }); | ||
| queryClient.invalidateQueries({ queryKey: courseOutlineQueryKeys.courseDetails(getCourseKey(itemId)) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with the courseDetail here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2830 (comment)
| /** | ||
| * Creates a resizable box that can be dragged to resize the width from the left side. | ||
| */ | ||
| export const ResizableBox = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!
| {/* eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex, jsx-a11y/no-static-element-interactions */} | ||
| <div className="resizable-handle" onMouseDown={onMouseDown} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to add a button/handler here. I just noticed that the sidebar was resizable after looking at the code.
Also, it should help with accessibility.
This could be discussed upstream and implemented in a follow-up task if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. However does the resizing really matter in accessibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I have no idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For people who can see fine but only use a keyboard and can't use a mouse, it may be nice if there were a way to resize using the keyboard. I don't think that's a requirement here though, just something that would be a nice enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradenmacdonald Makes sense.
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(contentId), | ||
| }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseDetails(courseKey), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as commented above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2830 (comment)
src/CourseAuthoringContext.tsx
Outdated
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(currentSelection?.sectionId), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we invalidating a section when opening a unit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was used to invalidate parent data after a new unit is created but not needed anymore as it is handled in the source.
| const showNewSidebar = getConfig().ENABLE_COURSE_OUTLINE_NEW_DESIGN?.toString().toLowerCase() === 'true'; | ||
| if (showNewSidebar) { | ||
| setCurrentPageKey('align', cardId); | ||
| setCurrentPageKey('align'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking on the taxonomy tag on the card header is crashing here:
TypeError
can't access property "component", pages[currentPageKey] is undefined
Call Stack
Sidebar
src/generic/sidebar/Sidebar.tsx:76:28
renderWithHooks
node_modules/react-dom/cjs/react-dom.development.js:15486:27
updateFunctionComponent
node_modules/react-dom/cjs/react-dom.development.js:19613:20
beginWork
node_modules/react-dom/cjs/react-dom.development.js:21636:16
callCallback
node_modules/react-dom/cjs/react-dom.development.js:4164:14
invokeGuardedCallbackDev
node_modules/react-dom/cjs/react-dom.development.js:4213:16
invokeGuardedCallback
node_modules/react-dom/cjs/react-dom.development.js:4277:31
beginWork$1
node_modules/react-dom/cjs/react-dom.development.js:27486:28
performUnitOfWork
node_modules/react-dom/cjs/react-dom.development.js:26592:12
workLoopSync
node_modules/react-dom/cjs/react-dom.development.js:26501:22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido This was a weird one. My guess is that getConfig doesn't have all values set at the top level (initial render), so it decides that taxonomy is not enabled and so align page is not included in sidebar.
I had to update sidebar page provider to set the pages value inside OutlineSidebarPagesProvider which is component and so getConfig returns correct values and the align page is included. See 112b256
But this change now forces using OutlineSidebarPagesProvider and also the user of this provider has to use pages props to update sidebar pages in plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
I don't think that we need this pages props here.
A plugin could override it by nesting another Provider, the way is shown in the comment before the context.
And that way, we can have more control at the plugin to also remove pages, change order, etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @navinkarkera!
I would prefer that we do not use the pages prop, so we could have more flexibility (removing pages, changing order, etc..) on the plugin end.
I ported your fix (removing the props) here: 10b24e8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Updated here: ee9fa58
93f0d3a to
102034e
Compare
rpenido
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thank you for your work, @navinkarkera!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
Just left one comment about removing the pages props from the provider.
Yay, that's awesome! Is there any way to separate that into a separate PR or is it too tied in to the sidebar changes? I'm tracking the redux removal in #2540 - could you please let me know which parts of that list I can update / link to this PR ? |
| {icon && <Icon src={icon} className="mr-1 text-primary" size="sm" />} | ||
| {title && ( | ||
| <h3 className="h5 font-weight-bold text-primary mb-0"> | ||
| <h5 className="font-weight-bold text-primary mb-0 mt-1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, to improve accessibility and browsing the document via heading navigation, this should only be an <h5> if there is also an <h4>, <h3>, <h2>, and <h1> already on the page. So it should probably stay as an <h3> or perhaps even an <h2> (if there is no <h2> above), but it can be styled using the h5 class to look like an h5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, the sidebar has an h2 (title of sidebar) so making this h3 seems like the correct option.
ChrisChV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera It's an incredible job! Thanks for the refactoring! It looks good; I've left some comments. I want to test the code, but I'll do that tomorrow.
src/course-outline/data/slice.ts
Outdated
| }); | ||
| }, | ||
| // istanbul ignore next | ||
| addUnit: (state: CourseOutlineState, { payload }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera Is it absolutely necessary to add this? If it's too complex to remove, could you add a comment explaining how to remove it? We are actively working on removing React Redux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it is required, we cannot really remove it until we migrate the whole course outline to react-query. I'll add a comment here.
src/course-outline/hooks.jsx
Outdated
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(currentSelection.subsectionId), | ||
| }); | ||
| queryClient.invalidateQueries({ | ||
| queryKey: courseOutlineQueryKeys.courseItemId(currentSelection.sectionId), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera Thank you for all this refactoring! I know it's tiring work. We are currently migrating Redux to React Query in #2540. If you have extra CC hours, you can continue this work there.
ee9fa58 to
8a915c4
Compare
@bradenmacdonald Unfortunately it is not easy to separate them. I had to refactor mostly due to sync and access issues inside sidebar without fetching data multiple times.
This PR migrates some parts of |
|
@navinkarkera I found an issue with the "Unlink from library" for units. It doesn't happen in master |
4c07979 to
12b2569
Compare
ChrisChV
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@navinkarkera Looks good! Thanks!
Description
screencast.mp4
Following items are not implemented:
Apologies for the huge number of changes, could not help it due to migrating parts to react-query
Supporting information
Private-ref: https://tasks.opencraft.com/browse/FAL-4293Private-ref: https://tasks.opencraft.com/browse/FAL-4296Testing instructions
Other information
Include anything else that will help reviewers and consumers understand the change.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'